ARROW-7058: [C++] FileSystemDataSourceDiscovery should apply partition schemes relative to its base dir #5772
Conversation
… schemes relative to its base dir
fsaintjacques
left a comment
fsaintjacques
left a comment
There was a problem hiding this comment.
From an API perspective, this is the wrong approach. The caller can't disable this non-trivially (passing a selector with an empty base is... an implementation detail hack) and forces the caller to always pass a Selector even if there's none, e.g how can the user pass just an explicit list of FileStats?
This should live in PartitionScheme, independent of DataSourceDiscovery, Either via a prefix optional parameter or a composed StripBasePartitionScheme(child_scheme, prefix) if you don't want to add the prefix to all relevant schemes.
@nealrichardson any idea on how to make this ergonomic?
fsaintjacques
changed the title
|
IDK about C++ API ergonomics, I just know what the expected behavior should be. If I tell you my dataset is in |
| std::vector<fs::FileStats> files, PathPartitions* out) { | ||
| for (const auto& file : files) { | ||
| const auto& path = file.path(); | ||
| if (file.path().substr(0, base_dir.size()) != base_dir) continue; |
There was a problem hiding this comment.
We need to decide what should be the most friendly behavior in such case. I'd tempted to vote for parse without trimming instead of skipping the file.
std::string path = file.path();
if (!base_dir.empty() && path.substr(0, base_dir.size()) == base_dir) {
path = path.substr(base_dir.size());
}There was a problem hiding this comment.
Parsing some paths with a known prefix removed and other paths with an unknown prefix seems like a foot gun to me. I'd expect the files which lie outside base_dir to be picked up with a different instance of Discovery (with the correct base_dir for those files).
There was a problem hiding this comment.
I agree with Ben. If I'm supplying my own file paths, and some are relative to the given "base_dir" and some aren't, (1) the simple partition scheme of applying a schema to path segments probably won't just work across all of them if they don't share a base_dir, and if I want to use it I should put them in different Discovery instances; and (2) if we tried to get partition info from files outside of base_dir, it will probably error in parsing the path segments, so continue and not assign partition data to that file is probably safest.
3 participants
@nealrichardson